Skip to content

Add telemetry for retrievers #114109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 26 commits into from
Oct 10, 2024
Merged

Add telemetry for retrievers #114109

merged 26 commits into from
Oct 10, 2024

Conversation

pmpailis
Copy link
Contributor

@pmpailis pmpailis commented Oct 4, 2024

This is to update the information present in ClusterStats for retrievers. The proposed approach is to introduce a new element named retriever under sections that would compute just the top-level usages of a retriever, and additionally a new section named retrievers that would contain the names of all the retrievers used anywhere in the tree, i.e. all top-level usages of a retriever + any nested retrievers used (e.g. children of rrf or text_similarity_reranker)

So assuming that we run a knn retriever search, and an rrf with a standard and a knn as direct children, the suggested output would now be:

{
    "search":
    {
     ...
        "sections":                     
        {
            "retriever": 2             
        },
        "retrievers":                   
        {
            "rrf": 1,
            "knn": 2,
            "standard": 1
        }
    }
}

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 4, 2024

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

Hi @pmpailis, I've created a changelog YAML for you.

@pmpailis pmpailis marked this pull request as ready for review October 8, 2024 08:59
@pmpailis pmpailis added the auto-backport Automatically create backport pull requests when merged label Oct 8, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@elasticsearchmachine elasticsearchmachine added the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Oct 8, 2024
@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 8, 2024

@elasticmachine update branch

carlosdelest
carlosdelest approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@carlosdelest carlosdelest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Should we add an AbstractBWCSerializationTestCase now that the format has changed for ClusterStats?

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 8, 2024

Should we add an AbstractBWCSerializationTestCase now that the format has changed for ClusterStats?

The main change lies in how SearchUsageStats is structured, which is already covered by a bwc test (SearchUsageStatsTests#testSerializationBWC). I've updated it slightly in 215f120 to better reflect on the used TransportVersions.

@carlosdelest do you think that this would be enough, or should we add a new test class for the top level ClusterStatsResponse ?

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 8, 2024

@elasticmachine update branch

@carlosdelest
Copy link
Member

do you think that this would be enough

@pmpailis , absolutely! Thanks for pointing that out 👍

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 8, 2024

@elasticmachine update branch

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 8, 2024

run elasticsearch-ci/part-1

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 8, 2024

@elasticmachine update branch

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 9, 2024

run elasticsearch-ci/packaging-tests-windows-sample

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 9, 2024

@elasticmachine update branch

@pmpailis pmpailis merged commit 4eab631 into elastic:main Oct 10, 2024
16 checks passed
@pmpailis pmpailis mentioned this pull request Oct 10, 2024
Merged
pmpailis added a commit to pmpailis/elasticsearch that referenced this pull request Oct 10, 2024
@pmpailis
Add telemetry for retrievers (elastic#114109)
037299c
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.x

elasticsearchmachine pushed a commit that referenced this pull request Oct 10, 2024
@pmpailis
Add telemetry for retrievers (#114109) (#114474)
3256f8b
matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 10, 2024
@pmpailis @matthewabbott
Add telemetry for retrievers (elastic#114109)
4eff28c
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Oct 13, 2024
@pmpailis @davidkyle
Add telemetry for retrievers (elastic#114109)
aa0e5a2
@pmpailis pmpailis deleted the add_telemetry_for_retrievers branch May 27, 2025 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosdelest carlosdelest carlosdelest approved these changes

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged >enhancement :Search Relevance/Search Catch all for Search Relevance Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v8.16.0 v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pmpailis @elasticsearchmachine @carlosdelest @elasticmachine